Nested import ordering #236
Conversation
|
This has a few problems, do not merge. |
Had to edit a test that depended on the old, incorrect behavior
|
Fixed. @MoOx ? |
| var statements = parseStatements(result, styles) | ||
|
|
||
| return Promise.all(statements.map(function(stmt) { | ||
| return Promise.resolve(statements).then(promiseEach(function(stmt) { |
There was a problem hiding this comment.
what is the difference between promise each and Promise.all? Cause from the doc I can see
The array of values maintains the order of the original iterable object, not the order that the promises were resolved in.
There was a problem hiding this comment.
@MoOx I should have explained these changes better.
map runs the function down to return resolveImportId(). This starts a promise chain. map then goes on to the next item in statements, without waiting for resolveImportId to resolve. This causes concurrency. Depending on the order that the files are loaded, state is modified incorrectly. If foo-second.css loads first, bar.css is added to state.importedFiles. When foo-first.css is loaded, bar.css appears to be already loaded, however, it is in the incorrect order in the bundle.
promiseEach waits for resolveImportId to resolve before going to the next item in statements. promise-each is a standalone port of http://bluebirdjs.com/docs/api/promise.each.html.
This is a performance cut; but this is the quickest way to fix this. If someone can figure out a better deduping system that does not require serialized loading, great; I can't ATM.
Hope this is clear.
|
@RyanZim what is your npm nickname? |
|
You know have the full rights for this repo. To cut a release you will need to:
If you are not confortable doing this, I can do this with you if you want, or I can even do it alone, it will take me a minute. |
|
@MoOx ryanzim |
|
|
|
I think when there are fork, continuous-integration/appveyor/branch is incorrectly not working as expected. No big deal imo. |
Branch only runs on internal PRs. |
|
@MoOx I would like to do releases in the future; but can you ship this today? I don't have time ATM. |
|
I just released 8.1.3 |
|
Thanks! |
Had to edit a test that depended on the old, incorrect behavior.
Fixes #168 🎉